Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

constructor-level simplification #154

Merged
merged 29 commits into from
Jan 9, 2021
Merged

constructor-level simplification #154

merged 29 commits into from
Jan 9, 2021

Conversation

shashi
Copy link
Member

@shashi shashi commented Jan 4, 2021

This design is taken from SymEngine. It is similar to what I had going on here: https://github.com/shashi/ModelingToolkit.jl/blob/s/simplification/src/polyform.jl with optimizations for the simplest cases.

fixes #151

todo:

  • fix output symtype.

@shashi
Copy link
Member Author

shashi commented Jan 4, 2021

@YingboMa what was your benchmark big expression?

src/fast-terms.jl Outdated Show resolved Hide resolved
@YingboMa
Copy link
Member

YingboMa commented Jan 4, 2021

To generate a big expression, I recommend to use

julia> using LinearAlgebra, ModelingToolkit

julia> @variables X[1:5, 1:5];

julia> det(lu(X))

shashi and others added 3 commits January 4, 2021 18:40
…umber

Before:
```julia
julia> -1 + x + -1 + 1
1 + -1 + -1 + x
```

After:
```julia
julia> -1 + x + -1 + 1
-1 + x
```
@YingboMa
Copy link
Member

YingboMa commented Jan 5, 2021

@shashi there was

+(a::Number, b::Add) = iszero(a) ? b : Add(a, make_add_dict(1, b))

before and I changed it to

+(a::Number, b::Add) = iszero(a) ? b : Add(a + b.coeff, b.dict)

When b is already an Add does it still need make_add_dict(1, b)? I thought we only need make_add_dict(1, b) when b is not an Add to canonicalize it.

@YingboMa
Copy link
Member

YingboMa commented Jan 5, 2021

BTW, we used

macro bigexpr()
    ex = :(x + y + z + z * y + o * x + x)
    bigex = Expr(:call,)
    bigex.args = [:+; mapreduce(_->ex.args[2:end], vcat, 1:200)];
    esc(bigex)
end

to benchmark construction speed before.

SymEngine:

julia> using SymEngine

julia> @vars x y z o
(x, y, z, o)

julia> @time @bigexpr
  0.001075 seconds (2.80 k allocations: 62.609 KiB)
400*x + 200*y + 200*z + 200*o*x + 200*y*z

SymbolicUtils:

julia> using SymbolicUtils

julia> @syms x y z o
(x, y, z, o)

julia> @time @bigexpr
  0.001439 seconds (35.57 k allocations: 2.088 MiB)
200o * x + 200y + 200z + 200y * z + 400x

SymbolicUtils master:

julia> @time simplify(expr, polynorm=true)
  0.162938 seconds (3.59 M allocations: 64.250 MiB, 1.61% gc time)
(400 * x) + (200 * (y + z)) + (200 * o * x) + (200 * y * z)

PS: I did run twice to make sure that there's no compile time.

shashi and others added 4 commits January 5, 2021 07:09
Co-authored-by: "Yingbo Ma" <mayingbo5@gmail.com>
Co-authored-by: "Yingbo Ma" <mayingbo5@gmail.com>
Co-authored-by: "Yingbo Ma" <mayingbo5@gmail.com>
Co-authored-by: "Yingbo Ma" <mayingbo5@gmail.com>
@codecov-io
Copy link

codecov-io commented Jan 5, 2021

Codecov Report

Merging #154 (e2517f5) into master (e00284a) will increase coverage by 0.37%.
The diff coverage is 96.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
+ Coverage   91.55%   91.93%   +0.37%     
==========================================
  Files          11       11              
  Lines         687      818     +131     
==========================================
+ Hits          629      752     +123     
- Misses         58       66       +8     
Impacted Files Coverage Δ
src/SymbolicUtils.jl 100.00% <ø> (ø)
src/types.jl 91.63% <95.59%> (+5.92%) ⬆️
src/methods.jl 89.55% <100.00%> (+2.05%) ⬆️
src/rewriters.jl 88.00% <100.00%> (+1.23%) ⬆️
src/rule.jl 89.10% <100.00%> (-1.07%) ⬇️
src/ordering.jl 94.87% <0.00%> (-2.66%) ⬇️
src/matchers.jl 96.15% <0.00%> (-2.10%) ⬇️
src/simplify_rules.jl 81.81% <0.00%> (-1.52%) ⬇️
src/utils.jl 90.10% <0.00%> (-0.90%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e00284a...e2517f5. Read the comment docs.

@shashi
Copy link
Member Author

shashi commented Jan 5, 2021

fixes #151

@shashi
Copy link
Member Author

shashi commented Jan 8, 2021

@cscherrer @david-pl pinging you both since you have been using this library!

+, -, *, /, ^ on <:Symbolic now return Add, Mul or Pow which are subtype of Symbolic. All other previously defined functions still return Term.

The interface of operation(x) and arguments(x) is defined on all these types, and istree(x) = true for all of them.

If you never access the fields of a term or never dispatch on Term in your code, nothing should change for you.

Now instead of dispatching on Term, use the if istree(x) to also accept Add Mul and Pow... this should not have performance penalty, and it does not harm flexibility a lot since you can still also use dispatch.

EDIT: + and * on Symbolic{<:Any} are assumed to be commutative and associative by default ON construction. so you may want to define your own + or * methods for Symbolic{<:YourType} objects to return Term -- this would be the default before this change.

@rules should continue to work as they did before.

Hopefully this helps, feel free to ping me for help any time!

@shashi
Copy link
Member Author

shashi commented Jan 8, 2021

  • and * on Symbolic{<:Any} are assumed to be commutative and associative by default

This should have been restricted to Symbolic{<:Number}, but see the issue here #155 which makes that not yet feasible...

@cscherrer
Copy link
Contributor

cscherrer commented Jan 8, 2021

Thanks @shashi! This reminds me of a question. It's a little bit of a tangent, so I'll ask in the Zulip and edit this message with a link.

EDIT: Here it is: https://julialang.zulipchat.com/#narrow/stream/236639-symbolic-programming/topic/Constructor-level.20simplification/near/222106581

@TymonKilich
Copy link
Contributor

TymonKilich commented Mar 4, 2021

  • and * on Symbolic{<:Any} are assumed to be commutative and associative by default

This should have been restricted to Symbolic{<:Number}, but see the issue here #155 which makes that not yet feasible...

It certainly surprised me, so maybe I should put a reference to this PR with information about that behaviour in manual/page?

How are the perspectives for solving this issue looking? I'm wondering whether to put a note about that behaviour in manual as for me it's certainly surprising that e.g.

julia> @syms f() g()
(f()::Number, g()::Number)

julia> 2g + g*f + f*g + g
3g()::Number + 2f()::Number*g()::Number

of course I suppose that it's mostly ok for the functions that return numbers (at least can't think of the reason why it wouldn't be ATM), since * ≢ ∘ but is still a bit of surprise 😄

@shashi shashi changed the title WIP: constructor-level simplification constructor-level simplification May 8, 2021
@shashi
Copy link
Member Author

shashi commented Aug 5, 2021

This should have been restricted to Symbolic{<:Number}, but see the issue here #155 which makes that not yet feasible...

How are the perspectives for solving this issue looking? I'm wondering whether to put a note about that behaviour in manual as for me it's certainly surprising that e.g.

For those looking at this now, it was fixed in #259 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify + and * on construction when the type is commutative and associative
6 participants